Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add exemplar support to Prometheus exporter #5929

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add exemplar support to Prometheus exporter #5929

wants to merge 5 commits into from
+1,129 −64

Conversation

saul
Copy link

@saul saul commented Oct 27, 2024 •
edited
Loading

Fixes #3449
Design discussion issue - not found

Changes

Adds support for exporting histogram exemplars to OpenTelemetry.Exporter.Prometheus.AspNetCore and OpenTelemetry.Exporter.Prometheus.HttpListener. There are a couple of aspects that I'd appreciate some guidance on:

  • The spec states "If no exemplars exist on a bucket, the highest exemplar from a lower bucket MUST be used, even though it is a duplicate of another bucket’s exemplar" but my implementation doesn't follow this. I'm wondering what the rationale is behind this as for large histograms I can imagine this would produce a lot of extraneous data.
  • The spec states "For Prometheus pull endpoints, only a single exemplar is able to be added to each bucket, so the largest exemplar from each bucket MUST be used, if attaching exemplars" but exemplars as implemented in OTel.Net do NOT preserve the largest exemplar in each bucket on histograms. Simply the most recent value for that bucket is stored as the exemplar, regardless of whether it is larger than the current exemplar or not
  • Do the exemplars need to be 'wiped' between scrapes of the /metrics endpoint? Or should the exemplars be persistent in that once a bucket has an exemplar, it will always be returned by the /metrics endpoint until it is replaced by another exemplar.
  • Is this considered an API-breaking change? I wonder if it would be best to add a bool option to PrometheusAspNetCoreOptions to enable exemplars, with the default being disabled (i.e. current behaviour)
    • "Prometheus Exporters are not stable yet, so no concern about breaking changes here." @cijothomas

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable) - see question above re adding a PrometheusAspNetCoreOptions option

@saul saul requested a review from a team as a code owner October 27, 2024 11:17
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Oct 27, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot added pkg:OpenTelemetry.Exporter.Prometheus.AspNetCore Issues related to OpenTelemetry.Exporter.Prometheus.AspNetCore NuGet package pkg:OpenTelemetry.Exporter.Prometheus.HttpListener Issues related to OpenTelemetry.Exporter.Prometheus.HttpListener NuGet package labels Oct 27, 2024
@saul saul mentioned this pull request Oct 27, 2024
Open
@cijothomas
Copy link
Member

Is this considered an API-breaking change? I wonder if it would be best to add a bool option to PrometheusAspNetCoreOptions to enable exemplars, with the default being disabled (i.e. current behaviour)

I think you can enable it by default, given Exemplars itself is off-by-default in the SDK! So if prometheus exporter is exporting exemplars, users has already made an explicit decision to enable it at the sdk.
Another thing - Prometheus Exporters are not stable yet, so no concern about breaking changes here.

@cijothomas
Copy link
Member

CLA Not Signed

@saul Can you sign the CLA part, as it is a mandatory pre-req for accepting any contributions to OpenTelemetry. If you are working on behalf on an employer, you may want to consult your employer before CLA. (Note: This is a one time requirement only)

@CodeBlanch CodeBlanch marked this pull request as draft October 28, 2024 16:58
@saul saul force-pushed the aspnet-prom-exemplars branch from a609c15 to b412857 Compare October 29, 2024 10:47
@saul saul changed the title Add histogram exemplar support to Prometheus exporter Add exemplar support to Prometheus exporter Oct 29, 2024
@saul saul force-pushed the aspnet-prom-exemplars branch 2 times, most recently from d6e35f6 to bfedbfc Compare October 29, 2024 13:23
@saul saul force-pushed the aspnet-prom-exemplars branch from bfedbfc to 8550c31 Compare October 29, 2024 13:37
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 6, 2024

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added Stale Issues and pull requests which have been flagged for closing due to inactivity and removed Stale Issues and pull requests which have been flagged for closing due to inactivity labels Nov 6, 2024
@saul saul marked this pull request as ready for review November 12, 2024 09:33
@saul
Copy link
Author

saul commented Nov 12, 2024

Hi @cijothomas the CLA has been signed and this is ready for review when you have time. Thanks

@github-actions GitHub Actions
Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Nov 20, 2024
@Kielek Kielek requested a review from cijothomas November 20, 2024 12:48
@cijothomas
Copy link
Member

@robertcoltheart Tagging you here, to see if you have some bandwidth to help review this PR, given you have made several contributions to the Prometheus Exporter!

@cijothomas
Copy link
Member

The spec states "If no exemplars exist on a bucket, the highest exemplar from a lower bucket MUST be used, even though it is a duplicate of another bucket’s exemplar" but my implementation doesn't follow this. I'm wondering what the rationale is behind this as for large histograms I can imagine this would produce a lot of extraneous data.

I am not sure of the reasoning for this either...You can put a TODO in the code and state this is not implemented, and we can follow up with the spec.

@github-actions github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Nov 21, 2024
robertcoltheart
robertcoltheart suggested changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@robertcoltheart robertcoltheart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a technical perspective, this seems fine. I can't comment on whether this meets the Otel specification to a degree that is satisfactory, or whether there should be additional things added. Ie. I haven't done a mapping to see if there are any gaps between the spec and this PR.

src/OpenTelemetry.Exporter.Prometheus.HttpListener/Internal/PrometheusSerializerExt.cs
return WriteExemplar(buffer, cursor, maxExemplar, metric.Name, isLong);
}

private static int WriteExemplar(byte[] buffer, int cursor, in Exemplar exemplar, string metricName, bool isLong)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this go into PrometheusSerializer.cs alongside the rest of the byte-writing code for metrics?

@github-actions GitHub Actions
Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Nov 29, 2024
@Kielek Kielek removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Nov 29, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 7, 2024

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Dec 7, 2024
@Kielek Kielek removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Dec 7, 2024
@github-actions GitHub Actions
Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Dec 15, 2024
@github-actions github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertcoltheart robertcoltheart robertcoltheart requested changes

@cijothomas cijothomas Awaiting requested review from cijothomas

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
pkg:OpenTelemetry.Exporter.Prometheus.AspNetCore Issues related to OpenTelemetry.Exporter.Prometheus.AspNetCore NuGet package pkg:OpenTelemetry.Exporter.Prometheus.HttpListener Issues related to OpenTelemetry.Exporter.Prometheus.HttpListener NuGet package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use OpenMetrics format for exposing exemplars
4 participants
@saul @cijothomas @robertcoltheart @Kielek